perf: apply high-performance-go guidelines across 5 hot paths (3-round review) - #688
Conversation
Audited the codebase against docs/development/high-performance-go.md
and fixed the five highest-impact violations identified by scanning
internal/ and rules/.
1. rename: map[int]bool → map[int]struct{} (guideline: map[K]struct{}
for sets). ValidRefDefBodyLines and contentBlockLines tracked
presence-only with bool values; struct{} removes the 1-byte value
overhead and makes the set semantics explicit. Updated callers in
lsp/rename.go and rename_test.go.
2. export: map[int]bool → map[int]struct{} for stripLines/bodyLines
(same guideline). stripDirectives, overlapsAny, and emitLines all
operated on presence-only maps; changed all three sites.
3. secreview/render: fmt.Sprintf(":%d") → strconv.Itoa (guideline:
strconv over fmt.Sprintf). strconv.Itoa is ~3× faster than
fmt.Sprintf for integer formatting because it skips reflection and
format-string parsing.
4. index/locate: eliminate string(lines[i]) per loop iteration in
enclosingListKey (guideline: stay in []byte). FindSubmatch([]byte)
replaces FindStringSubmatch(string(line)), removing one heap
allocation per scanned line in the upward-scan loop. Only the
matched key group is converted to string at the return point.
5. concisenessscoring: eliminate message += fmt.Sprintf pattern
(guideline: strings.Builder over +). A single conditional
fmt.Sprintf replaces the initial Sprintf followed by a
string-concatenation assignment, removing one heap allocation per
diagnostic when verbose cues are present.
All tests pass. mdsmith check . reports 0 failures.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QYub2iGP6CeeEFSfjV3Wv
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…rage Three test improvements from code-review round 1: - rule_test.go: replace vacuous assert.NotEmpty (which strings.Contains already implies non-empty) with a meaningful assert.Contains for "e.g.," so the assertion can actually go red if the format string loses its example section - rule_test.go: add TestCheck_NoCuesMessage to exercise the `if examples == ""` branch — verboseParagraph always produces cues so the base-message-only path was uncovered; fixes the Codecov patch check failure (40% → 100% on that branch) - locate_test.go: rename TestEnclosingListKey_NoStringAllocPerLine to TestEnclosingListKey_FindsParentKey; the test only asserts the return value, not allocation behaviour, so the old name was misleading Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QYub2iGP6CeeEFSfjV3Wv
Two follow-on fixes from code review round 2: - locate.go: piArgRE's `\s*$` suffix already strips trailing whitespace from capture group 2, so `bytes.TrimSpace(m[2])` was always a no-op; replace with plain `len(m[2]) == 0`. - concisenessscoring/rule_test.go: TestCheck_NoCuesMessage previously used a hardcoded MinScore of 0.50, which would break if the embedded model drifts. Rewrite to probe NewScorer() at runtime, assert len(Cues)==0 (skip if the model now sees cues), and set MinScore = scored.Conciseness + 0.10 so the threshold is always just above the actual score. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QYub2iGP6CeeEFSfjV3Wv
…seness rule Two fixes from code-review round 3: - rule_test.go: TestCheck_MessageNoConcatenationWhenExamplesPresent guarded the assert.Contains(msg, "e.g.,") behind `if strings.Contains(msg, "reduce verbose cues")`, making it a no-op in the exact regression it was meant to catch (message drops cue text → outer if is false → inner assert never runs). verboseParagraph() always produces cues, so assert both strings unconditionally. Remove the now-unused "strings" import. - rule.go: Replace the if/else with two identical fmt.Sprintf call sites (same three arguments, same format prefix) with a single call that takes a conditional cuesSuffix string. The base format string now lives in one place so it can't diverge between the cue and no-cue paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QYub2iGP6CeeEFSfjV3Wv
|
🟢 Merge Queue — picked up This PR is in the queue and will be batched with other Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run. |
|
🔵 Merge Queue — CI running Merged into batch branch Next: No action needed — you'll be notified when CI completes. |
|
✅ Merge Queue — merged This PR landed on Next: Done — nothing more to do here. |
Summary
Performance audit against
docs/development/high-performance-go.md, fixing 5 violations across core packages. Three rounds ofcode-review --fixat high severity were applied before merging.Fixes (commit
7c9dfc7)internal/export/export.gomap[int]boolsets for line trackingmap[int]struct{}— zero-size value, same semanticsinternal/index/locate.goenclosingListKeyFindStringSubmatch(string(lines[i]))— onestring()copy per scanned lineFindSubmatch(lines[i])— regex operates directly on[]byteinternal/lsp/rename.go/internal/rename/rename.goValidRefDefBodyLinesreturnedmap[int]bool; lookup usedmap[bool]zero-valuemap[int]struct{}; lookups use_, okidiominternal/secreview/render.golocStrfmt.Sprintf(":%d", n)— reflection overhead for integer formatting":" + strconv.Itoa(n)— ~3× faster, no reflectioninternal/rules/concisenessscoring/rule.gomessage += fmt.Sprintf(...)— intermediate string allocation on verbose pathfmt.Sprintfvia if/else — no extra allocationCode-review round 1 fixes (commit
917eafa)rule_test.goTestCheck_MessageNoConcatenationWhenExamplesPresent: replaced vacuousassert.NotEmptywithassert.Contains(msg, "e.g.,")— the original guard could never faillocate_test.go: renamedTestEnclosingListKey_NoStringAllocPerLine→TestEnclosingListKey_FindsParentKey(old name implied allocation testing the test didn't do)rule_test.go: addedTestCheck_NoCuesMessageto cover theexamples == ""branch, which was missing coverage (Codecov patch gate was failing at 82%)Code-review round 2 fixes (commit
12f6a79)locate.goenclosingListKey: removed redundantbytes.TrimSpace(m[2])— piArgRE's\s*$suffix structurally guarantees group 2 has no trailing whitespace; replaced withlen(m[2]) == 0rule_test.goTestCheck_NoCuesMessage: replaced hardcodedMinScore: 0.50(fragile against model drift) with a runtime scorer probe — callsNewScorer(), assertslen(Cues) == 0, and setsMinScore = scored.Conciseness + 0.10Code-review round 3 fixes (commit
3a45453)rule_test.goTestCheck_MessageNoConcatenationWhenExamplesPresent: removed conditional guardif strings.Contains(msg, "reduce verbose cues")— the innerassert.Contains(msg, "e.g.,")was dead code in the exact regression it was meant to catch (if implementation drops cue text, the outerifis false and the inner assert never runs);verboseParagraph()always produces cues so both asserts are now unconditionalrule.go: unified the twofmt.Sprintfcall sites (sharing the same base format and three repeated arguments) into a single call with a conditionalcuesSuffixstring — base format string now lives in one placeTest plan
go test ./...code-review --fixat high severity — no surviving CONFIRMED findings after round 3go vet ./...clean🤖 Generated with Claude Code
https://claude.ai/code/session_015QYub2iGP6CeeEFSfjV3Wv